-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BtcSwapTx
: update refund tx to consider all utxos
#73
BtcSwapTx
: update refund tx to consider all utxos
#73
Conversation
@i5hi as I'm still testing this, I marked it as a draft and wanted to ask for your high level thoughts and feedback:
|
The approach is good! We don't need the old |
The BtcSwapTx methods are updated to use all available inputs, where appropriate.
Alright, I continued with this approach and removed the Short summary of the changes:
I'm not fully sure about the parts marked with (*), more specifically:
What do you think @i5hi? Are my assumptions correct? |
Boltz will never lockup more than 1 utxo, so wont need to. However, for consistency the code should attempt to spend all utxos, incase for whatever reason there is more than 1 - no harm I suppose.
We refund what we lock* up - so incase we lock up more than 1 utxo, we must refund both. |
@michael1011 Need confirmation from you on this |
Infact, we should stick with your original approach of using while |
In theory, yes. But it's very likely never going to happen (unless there is a grave bug) that we lock more than one UTXO for a swap.
Yes. When asking for a cooperative refund signature, one of the parameters is |
This preserves the existing logic of when to fallback to querying Boltz for the utxo.
Add new argument for get_chain_partial_sig, get_submarine_partial_sig that represent the input index.
@i5hi @michael1011 thanks for your feedback. I addressed your points and added one question about |
@ok300 Have you tried running the integration submarine tests and send 2 payments to the address to see if this all works well? |
Also ensure that standard refunds are working as expected |
@i5hi I ran Are these the tests you mean and if so, what's the proper way to run them? Just change the invoice and refund address fields, then remove the |
you dont need to remove ignore, for each submarine test you have to use a fresh invoice or you will get a 400 from boltz:
|
I tried Do I have to set anything else? |
Just tried a pheonix testnet invoice; worked fine. Try this
|
I ran two kinds of tests: First, I used this branch to trigger (and complete) refunds for Incoming Chain Swaps (BTC -> LBTC) in breez/breez-sdk-liquid#471 . Both cooperative and non-cooperative claims (to LBTC, as final step of the chain swap) and refunds (to BTC) were successful. Second, I used @i5hi 's suggestion above to run the The PR is ready for final review 🙏 |
@michael1011 @i5hi anything else I can do to help with the review? We are using this branch in the Liquid Breez SDK and it would make life easier to get it merged 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK
Thanks @ok300 |
This PR updates
BtcSwapTx
and itsnew_refund
method to consider all available utxos.Fixes #72